Skip to content

Connect to dynamic simulation API with full parameters #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

thangqp
Copy link
Contributor

@thangqp thangqp commented Nov 29, 2022

  • Provide DynamicMappingClient to communicate to the dynamic-mapping-server in order to get model groovy and parameters par
  • Provide TimeSeriesClient to communicate to the timeseries-server in order to send timeseries/timeline
  • Provide an internal ParametersService in order to manipulate and copy reference parameters files from resources/parameters directory to a local directory, i.e. /tmp/dynamic_simulation_xxx
  • Provide NotificationService for notifying messages
  • Database, persist simulation result as : resultUuid | timeSeriesUuid | timeLineUuid | status IN result table
  • Writing tests for all services and controllers and a merge .par files utile, i.e. XmlMerge

@thangqp thangqp added the WIP label Nov 29, 2022
@thangqp thangqp force-pushed the branching_to_dynamic_simulation_API_with_full_parameters branch from c8db96d to 7f769f8 Compare December 9, 2022 14:29
notificationService.emitResultDynamicSimulationMessage(sendMessage);
LOGGER.info("Dynamic simulation complete (resultUuid='{}')", resultContext.getResultUuid());
})
.toFuture().get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to investigate, at least put a big comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, I put only more comments

@@ -5,7 +5,7 @@
<column name="result_uuid" type="UUID">
<constraints nullable="false" primaryKey="true" primaryKeyName="resultPK"/>
</column>
<column name="result" type="BOOLEAN"/>
<column name="result" type="UUID"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not a separate changeset ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

debug: true

dynawaltz-default-parameters:
parametersFile: /home/phamquy/Projects/dynawo/test/models.par
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this used ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, in fact there is a service, i.e. ParametersService, which configures parameters for Dynawaltz. This service does not use "dynawaltz-default-parameters".
I will remove all params in "dynawaltz-default-parameters"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,11 @@
dynawaltz:
homeDir: /home/runner/work/dynamic-simulation-server/dynawo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this used ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, homDir is mandatory and loaded by DynaWaltzConfig for the DynaWaltzProvider.

In prod env, config.yml is in the config directory ~/.itool

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the next step, we will test with container.. so this config can be removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it used by Github CI ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes! it is used when launching tests.

/**
* @author Mathieu Bague <[email protected]>
*/
public class GroovyCurvesSupplier implements CurvesSupplier {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove and use core one when switching to 5.1.0

Copy link
Contributor Author

@thangqp thangqp Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted by add comment into code

/**
* @author Marcos de Miguel <demiguelm at aia.es>
*/
public class GroovyEventModelsSupplier implements EventModelsSupplier {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove and use core one when switching to 5.1.0

Copy link
Contributor Author

@thangqp thangqp Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted by add comment into code

RUNNING,
COMPLETED
CONVERGED,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To check but I think SUCCEED and FAILED is more accurate than CONVERGED and DIVERGED for a dynamic simulation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we can see that later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, CONVERGED and DIVERGED are two sub-states of SUCCEED. In contrast, FAILED is another state which relates to a technical issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to discuss later


private String name;

private String parentName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the purposes of parentName and current?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Dto is from the one in dynamic-mapping-server, so may be some fields are not really useful in dynamic-simulation-server.
'parentName' is the name of mapping
'name' is the name of script given by dynamic-mapping-server during the generation
In fact, these two names are not used for the computation but may be useful for the log, so I will keep these fields.

The field 'current', as I see in the implementation ScriptServiceImpl#isScriptCurrent(Script) in dynamic-mapping-server, is used to say that the script generated at some moment is out of date or not (in comparing to the modified date of the mapping). This field is not used in dynamic-simulation-server since the script is always generated on-demand.

I will remove this field in this Dto in dynamic-simulation-server

@@ -45,25 +59,42 @@ private static String getNonNullHeader(MessageHeaders headers, String name) {

public static DynamicSimulationResultContext fromMessage(Message<String> message) {
Objects.requireNonNull(message);

String parametersPayload = message.getPayload();
ByteArrayInputStream bytesIS = new ByteArrayInputStream(parametersPayload.getBytes());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UTF-8?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simplicity, I will change the code by typing the payload as byte[] instead of currently as String to avoid a wasting step of conversion to String.

@@ -0,0 +1,281 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

@geofjamg geofjamg Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file useful ? already in resources in src/test/resources/data/ieee14/_01/input/models.par ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same name but the content may be different. It is used for testing the xml merge tool. In my opinion, it is better to separate test data.. So I will keep this file..

Anyway, the merge tool will be removed when the multi input-files implementation at dynawo finished

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

private void sendResultMessage(Message<String> message) {
OUTPUT_MESSAGE_LOGGER.debug("Sending message : {}", message);
publishResult.send("publishResult-out-0", message);
List<TimeSeries> timeSeries = new ArrayList(result.getCurves().values());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new ArrayList<>(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


// prepare expected result to compare
DynamicSimulationResult expectedResult = DynamicSimulationResultDeserializer.read(getClass().getResourceAsStream(Paths.get(DATA_IEEE14_BASE_DIR, testBaseDir, OUTPUT, RESULT_JSON).toString()));
String jsonExpectedTimeSeries = TimeSeries.toJson(new ArrayList(expectedResult.getCurves().values()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new ArrayList<>(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

String jsonResultTImeSeries = TimeSeries.toJson(TIME_SERIES_MOCK_BD.remove(timeSeriesUuid));

// compare result only timeseries
ObjectMapper mapper = new ObjectMapper();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inject Spring one instead of creating a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

true,
parametersFile);

ObjectWriter ow = new ObjectMapper().writer().withDefaultPrettyPrinter();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inject Spring object mapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

2 - Partial implementation 'stop' endpoint
@jonenst
Copy link
Contributor

jonenst commented Jan 26, 2023

add a comment in the code for all the things that must be deleted in the near future (config.yml /home/runner, groovy stuff copypasted)

@thangqp
Copy link
Contributor Author

thangqp commented Jan 26, 2023

add a comment in the code for all the things that must be deleted in the near future (config.yml /home/runner, groovy stuff copypasted)

Done!

XmlMerge xmlMerge = new SimpleXmlMerge();
try {
Document mergedDoc = xmlMerge.merge(dynamicParamsIs, eventsParamsIs);
xmlMerge.export(mergedDoc, Files.newOutputStream(tmpDir.resolve(modelParFileName)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Output stream should be closed

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 12 Code Smells

86.3% 86.3% Coverage
0.0% 0.0% Duplication

@geofjamg geofjamg merged commit 6ddc839 into main Jan 30, 2023
@geofjamg geofjamg deleted the branching_to_dynamic_simulation_API_with_full_parameters branch January 30, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants